fix(read only folder): allow renaming new file inside read-only folders

needed to download a new file inside a read-only folder

Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>

diff --git a/src/common/filesystembase.cpp b/src/common/filesystembase.cpp
index 660e2d382c648c5b1a33b89b94fac1dc681256d7..9cae216b0bac9f34abe8a25c285f5eba5be54e9e 100644 (file)
--- a/src/common/filesystembase.cpp
+++ b/src/common/filesystembase.cpp
@@ -245,54 +245,6 @@ bool FileSystem::rename(const QString &originFileName,
     return success;
 }
 
-bool FileSystem::uncheckedRenameReplace(const QString &originFileName,
-    const QString &destinationFileName,
-    QString *errorString)
-{
-#ifndef Q_OS_WIN
-    bool success = false;
-    QFile orig(originFileName);
-    // We want a rename that also overwrites.  QFile::rename does not overwrite.
-    // Qt 5.1 has QSaveFile::renameOverwrite we could use.
-    // ### FIXME
-    success = true;
-    bool destExists = fileExists(destinationFileName);
-    if (destExists && !QFile::remove(destinationFileName)) {
-        *errorString = orig.errorString();
-        qCWarning(lcFileSystem) << "Target file could not be removed.";
-        success = false;
-    }
-    if (success) {
-        success = orig.rename(destinationFileName);
-    }
-    if (!success) {
-        *errorString = orig.errorString();
-        qCWarning(lcFileSystem) << "Renaming temp file to final failed: " << *errorString;
-        return false;
-    }
-
-#else //Q_OS_WIN
-    // You can not overwrite a read-only file on windows.
-    if (!isWritable(destinationFileName)) {
-        setFileReadOnly(destinationFileName, false);
-    }
-
-    BOOL ok = 0;
-    QString orig = longWinPath(originFileName);
-    QString dest = longWinPath(destinationFileName);
-
-    ok = MoveFileEx((wchar_t *)orig.utf16(),
-        (wchar_t *)dest.utf16(),
-        MOVEFILE_REPLACE_EXISTING + MOVEFILE_COPY_ALLOWED + MOVEFILE_WRITE_THROUGH);
-    if (!ok) {
-        *errorString = Utility::formatWinError(GetLastError());
-        qCWarning(lcFileSystem) << "Renaming temp file to final failed: " << *errorString;
-        return false;
-    }
-#endif
-    return true;
-}
-
 bool FileSystem::openAndSeekFileSharedRead(QFile *file, QString *errorOrNull, qint64 seek)
 {
     QString errorDummy;

diff --git a/src/common/filesystembase.h b/src/common/filesystembase.h
index 8f5554000e079c28172aa666abfcc1d762f5b12a..e9547d2d0a7b66c05ba7e16f9f54cde61bcb79f2 100644 (file)
--- a/src/common/filesystembase.h
+++ b/src/common/filesystembase.h
@@ -132,14 +132,6 @@ namespace FileSystem {
         const QString &destinationFileName,
         QString *errorString = nullptr);
 
-    /**
-     * Rename the file \a originFileName to \a destinationFileName, and
-     * overwrite the destination if it already exists - without extra checks.
-     */
-    bool OCSYNC_EXPORT uncheckedRenameReplace(const QString &originFileName,
-        const QString &destinationFileName,
-        QString *errorString);
-
     /**
      * Removes a file.
      *

diff --git a/src/libsync/filesystem.cpp b/src/libsync/filesystem.cpp
index ac8a1814bcf844a28f7444bc5b1b24f05f19793c..002a031de3a5d66d8fa846aeefd1289f38a0b045 100644 (file)
--- a/src/libsync/filesystem.cpp
+++ b/src/libsync/filesystem.cpp
@@ -633,4 +633,52 @@ FileSystem::FilePermissionsRestore::~FilePermissionsRestore()
     }
 }
 
+bool FileSystem::uncheckedRenameReplace(const QString &originFileName, const QString &destinationFileName, QString *errorString)
+{
+#ifndef Q_OS_WIN
+    bool success = false;
+    QFile orig(originFileName);
+    // We want a rename that also overwrites.  QFile::rename does not overwrite.
+    // Qt 5.1 has QSaveFile::renameOverwrite we could use.
+    // ### FIXME
+    success = true;
+    bool destExists = fileExists(destinationFileName);
+    if (destExists && !QFile::remove(destinationFileName)) {
+        *errorString = orig.errorString();
+        qCWarning(lcFileSystem) << "Target file could not be removed.";
+        success = false;
+    }
+    if (success) {
+        success = orig.rename(destinationFileName);
+    }
+    if (!success) {
+        *errorString = orig.errorString();
+        qCWarning(lcFileSystem) << "Renaming temp file to final failed: " << *errorString;
+        return false;
+    }
+#else //Q_OS_WIN
+    const auto originFileInfo = QFileInfo{originFileName};
+    const auto originParentFolderPath = originFileInfo.dir().absolutePath();
+    FilePermissionsRestore renameEnabler{originParentFolderPath, FileSystem::FolderPermissions::ReadWrite};
+    // You can not overwrite a read-only file on windows.
+    if (!isWritable(destinationFileName)) {
+        setFileReadOnly(destinationFileName, false);
+    }
+
+    BOOL ok = 0;
+    QString orig = longWinPath(originFileName);
+    QString dest = longWinPath(destinationFileName);
+
+    ok = MoveFileEx((wchar_t *)orig.utf16(),
+                    (wchar_t *)dest.utf16(),
+                    MOVEFILE_REPLACE_EXISTING + MOVEFILE_COPY_ALLOWED + MOVEFILE_WRITE_THROUGH);
+    if (!ok) {
+        *errorString = Utility::formatWinError(GetLastError());
+        qCWarning(lcFileSystem) << "Renaming temp file to final failed: " << *errorString;
+        return false;
+    }
+#endif
+    return true;
+}
+
 } // namespace OCC

diff --git a/src/libsync/filesystem.h b/src/libsync/filesystem.h
index 26c999994394f48bd70eb88f5f51b9324b0267e3..2682f180b84d5a2d36cd628232e1eb38d7b07560 100644 (file)
--- a/src/libsync/filesystem.h
+++ b/src/libsync/filesystem.h
@@ -130,6 +130,14 @@ namespace FileSystem {
                                                   FileSystem::FolderPermissions permissions) noexcept;
 
     bool OWNCLOUDSYNC_EXPORT isFolderReadOnly(const std::filesystem::path &path) noexcept;
+
+    /**
+     * Rename the file \a originFileName to \a destinationFileName, and
+     * overwrite the destination if it already exists - without extra checks.
+     */
+    bool OWNCLOUDSYNC_EXPORT uncheckedRenameReplace(const QString &originFileName,
+                                                    const QString &destinationFileName,
+                                                    QString *errorString);
 }
 
 /** @} */

diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp
index 8e457e0b65c66e466d061b0c66fc2c7f497dcfd2..39aeb8589e0f0a622311feed365031374d0d2fab 100644 (file)
--- a/test/testpermissions.cpp
+++ b/test/testpermissions.cpp
@@ -45,6 +45,11 @@ static void assertCsyncJournalOk(SyncJournalDb &journal)
 #endif
 }
 
+static bool isReadOnlyFolder(const std::wstring &path)
+{
+    return FileSystem::isFolderReadOnly(std::filesystem::path{path});
+}
+
 SyncFileItemPtr findDiscoveryItem(const SyncFileItemVector &spy, const QString &path)
 {
     for (const auto &item : spy) {
@@ -644,27 +649,21 @@ private slots:
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
 
-        auto folderStatus = std::filesystem::status(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString());
-        QVERIFY(folderStatus.permissions() & std::filesystem::perms::owner_read);
-        QVERIFY(!static_cast<bool>(folderStatus.permissions() & std::filesystem::perms::owner_write));
+        QVERIFY(isReadOnlyFolder(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString()));
 
         remote.find("testFolder")->permissions = RemotePermissions::fromServerString("CKWDNVRSM");
 
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
 
-        folderStatus = std::filesystem::status(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString());
-        QVERIFY(folderStatus.permissions() & std::filesystem::perms::owner_read);
-        QVERIFY(folderStatus.permissions() & std::filesystem::perms::owner_write);
+        QVERIFY(!isReadOnlyFolder(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString()));
 
         remote.find("testFolder")->permissions = RemotePermissions::fromServerString("M");
 
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
 
-        folderStatus = std::filesystem::status(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString());
-        QVERIFY(folderStatus.permissions() & std::filesystem::perms::owner_read);
-        QVERIFY(!static_cast<bool>(folderStatus.permissions() & std::filesystem::perms::owner_write));
+        QVERIFY(isReadOnlyFolder(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString()));
     }
 
     void testChangePermissionsForFolderHierarchy()
@@ -688,15 +687,9 @@ private slots:
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
 
-        auto testFolderStatus = std::filesystem::status(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString());
-        QVERIFY(testFolderStatus.permissions() & std::filesystem::perms::owner_read);
-        QVERIFY(!static_cast<bool>(testFolderStatus.permissions() & std::filesystem::perms::owner_write));
-        auto subFolderReadWriteStatus = std::filesystem::status(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder/subFolderReadWrite")).toStdWString());
-        QVERIFY(subFolderReadWriteStatus.permissions() & std::filesystem::perms::owner_read);
-        QVERIFY(subFolderReadWriteStatus.permissions() & std::filesystem::perms::owner_write);
-        auto subFolderReadOnlyStatus = std::filesystem::status(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder/subFolderReadOnly")).toStdWString());
-        QVERIFY(subFolderReadOnlyStatus.permissions() & std::filesystem::perms::owner_read);
-        QVERIFY(!static_cast<bool>(subFolderReadOnlyStatus.permissions() & std::filesystem::perms::owner_write));
+        QVERIFY(isReadOnlyFolder(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString()));
+        QVERIFY(!isReadOnlyFolder(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder/subFolderReadWrite")).toStdWString()));
+        QVERIFY(isReadOnlyFolder(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder/subFolderReadOnly")).toStdWString()));
 
         remote.find("testFolder/subFolderReadOnly")->permissions = RemotePermissions::fromServerString("CKWDNVRSm");
         remote.find("testFolder/subFolderReadWrite")->permissions = RemotePermissions::fromServerString("m");
@@ -708,12 +701,9 @@ private slots:
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
 
-        subFolderReadWriteStatus = std::filesystem::status(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder/subFolderReadWrite")).toStdWString());
-        QVERIFY(subFolderReadWriteStatus.permissions() & std::filesystem::perms::owner_read);
-        QVERIFY(!static_cast<bool>(subFolderReadWriteStatus.permissions() & std::filesystem::perms::owner_write));
-        subFolderReadOnlyStatus = std::filesystem::status(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder/subFolderReadOnly")).toStdWString());
-        QVERIFY(subFolderReadOnlyStatus.permissions() & std::filesystem::perms::owner_read);
-        QVERIFY(subFolderReadOnlyStatus.permissions() & std::filesystem::perms::owner_write);
+        QVERIFY(isReadOnlyFolder(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString()));
+        QVERIFY(isReadOnlyFolder(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder/subFolderReadWrite")).toStdWString()));
+        QVERIFY(!isReadOnlyFolder(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder/subFolderReadOnly")).toStdWString()));
 
         remote.rename("testFolder/subFolderReadOnly", "testFolder/subFolderReadWriteNew");
         remote.rename("testFolder/subFolderReadWrite", "testFolder/subFolderReadOnlyNew");
@@ -722,9 +712,7 @@ private slots:
 
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
-        testFolderStatus = std::filesystem::status(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString());
-        QVERIFY(testFolderStatus.permissions() & std::filesystem::perms::owner_read);
-        QVERIFY(!static_cast<bool>(testFolderStatus.permissions() & std::filesystem::perms::owner_write));
+        QVERIFY(isReadOnlyFolder(static_cast<QString>(fakeFolder.localPath() + QStringLiteral("/testFolder")).toStdWString()));
     }
 
     void testDeleteChildItemsInReadOnlyFolder()